fix: use removeprefix to decode DynamoDB namespace property keys - #3602
Conversation
|
Do we even need tests? I don't think it is our job to test that the Additionally, this abstraction over a builtin is only used in one place, would it be simpler to just use |
Use str.removeprefix directly when decoding namespace properties so the literal p. prefix is removed without stripping leading property-key characters. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
f3e94aa to
4f45ca5
Compare
|
@jayceslesar updated as suggested: removed the one-use helper and builtin-focused tests, then called |
Fokko
left a comment
There was a problem hiding this comment.
Less is more, thanks @anxkhn for cleaning this up, and thanks @jayceslesar and @ebyhr for the review 🚀
Rationale for this change
DynamoDbCatalogstores each namespace property under a key prefixed withPROPERTY_KEY_PREFIX("p.") via_add_property_prefix, and decodes it on readwith
_remove_property_prefix:str.lstriptreats its argument as a set of characters to strip, not a literalprefix. With
PROPERTY_KEY_PREFIX == "p."it removes every leadingpand.from the decoded key, so any namespace property whose name starts with one of
those characters is corrupted when read back through
load_namespace_properties:providerroviderpathathpartitionartitionppp.hiddenhiddenupdate_namespace_propertiesreads the same decoded properties, so it operates onthe corrupted keys too. Property names that do not start with
por.(such ascomment,owner,location) round-trip correctly, which is why the existingtests, whose fixtures avoid those leading characters, never surfaced the bug.
The fix uses
str.removeprefix, which strips only the literal"p."prefix. Thesingle caller (
_get_namespace_properties) already guards each key withstartswith(PROPERTY_KEY_PREFIX), so behavior for every valid key is unchangedand only the previously corrupted keys are affected.
str.removeprefixisavailable in Python 3.9+ and this project requires
>=3.10.Are these changes tested?
Yes. Added
test_load_namespace_properties_with_prefix_char_keysintests/catalog/test_dynamodb.py, acreate_namespace/load_namespace_propertiesround trip (backed by
moto'smock_aws) over keysprovider,path,ppp,.hidden, andcomment. The test fails on the previouslstripimplementation(keys come back corrupted) and passes with
removeprefix.tests/catalog/test_dynamodb.pypasses (40 tests), andmake lint(ruff,ruff-format, mypy, and the other pre-commit hooks) is clean. The DynamoDB catalog
has no separate integration test path; behavior is covered by the mocked unit test.
Are there any user-facing changes?
Yes: a bug fix. Reading namespace properties from a DynamoDB-backed catalog now
returns property keys that begin with
por.correctly instead of truncatingtheir leading characters. No public API changes.